Skip to content

Comments

feat(calc): formulas fallback + 18 new builtins (lookup & conditional)#3

Merged
wolfiesch merged 3 commits intomainfrom
feat/formulas-integration
Feb 20, 2026
Merged

feat(calc): formulas fallback + 18 new builtins (lookup & conditional)#3
wolfiesch merged 3 commits intomainfrom
feat/formulas-integration

Conversation

@wolfiesch
Copy link
Collaborator

@wolfiesch wolfiesch commented Feb 20, 2026

Summary

Commit 1: formulas library fallback + 10 builtins

  • Wire formulas library as fallback for functions not in builtins (PMT, SLN, NPV, VLOOKUP, etc.)
  • 10 new builtins: ROUNDDOWN, MOD, POWER, SQRT, SIGN, LEFT, RIGHT, MID, LEN, CONCATENATE
  • range_shape() utility for 2D range metadata

Commit 2: 8 lookup & conditional aggregation builtins

  • 8 new builtins: INDEX, MATCH, XLOOKUP, CHOOSE, SUMIF, SUMIFS, COUNTIF, COUNTIFS
  • RangeValue dataclass: shape-aware 2D range container preserving (n_rows, n_cols) while staying iterable for backward compat
  • & string concatenation operator: enables =SUMIF(A1:A5, ">"&B1, C1:C5) dynamic criteria
  • Criteria matching engine: shared parser for operator prefixes, wildcards, and exact match
  • Whitelist: 39 → 43 | Builtins: 25 → 33

Why

These functions are required for LRBench-Agent scoring where agents build spreadsheets with formulas. The nested INDEX(range, MATCH(...)) pattern is ubiquitous in financial models and needs both functions as builtins.

Test plan

  • 298 total tests pass (255 existing + 43 new)
  • New tests cover all 8 builtins, & operator, criteria engine, RangeValue backward compat, and perturbation propagation
  • SynthGL perturbation verifier (12 tests) still passes

🤖 Generated with Claude Code

Wire the `formulas` library as a formula-level fallback for functions
not covered by builtins (financial: PMT, SLN, NPV; lookup: VLOOKUP,
HLOOKUP, INDEX/MATCH; etc). Multi-column ranges are reshaped to 2D
numpy arrays so lookup functions receive proper table_array shapes.

Add 10 new builtin implementations (25 total):
- Math: ROUNDDOWN, MOD, POWER, SQRT, SIGN
- Text: LEFT, RIGHT, MID, LEN, CONCATENATE

Add range_shape() utility for computing (n_rows, n_cols) from range
references. 28 new tests covering builtins, fallback, perturbation
propagation, and combined evaluation chains.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 20, 2026 09:02
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a two-tier formula evaluation strategy to the wolfxl calc engine: 25 builtin function implementations for fast, deterministic evaluation, plus a fallback to the formulas library for 200+ Excel functions not covered by builtins. The PR adds 10 new builtin functions (ROUNDDOWN, MOD, POWER, SQRT, SIGN, LEFT, RIGHT, MID, LEN, CONCATENATE) and introduces infrastructure to compile and cache formulas library functions with proper cell reference resolution and numpy array reshaping for multi-dimensional ranges.

Changes:

  • Added 10 new builtin implementations for math and text functions (total 25 builtins)
  • Integrated formulas library as lazy fallback with compilation caching and parameter resolution
  • Added range_shape utility to compute dimensions for multi-column range reshaping
  • Added 28 comprehensive integration tests covering builtins, fallback with constants, fallback with cell references, perturbation propagation, and combined scenarios

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.

File Description
tests/test_calc_formulas_integration.py New test file with 28 tests covering all 10 new builtins and formulas library fallback scenarios
python/wolfxl/calc/_parser.py Added range_shape utility to compute (rows, cols) for range references
python/wolfxl/calc/_functions.py Added 10 new builtin implementations (5 math, 5 text) and registered them in _BUILTINS
python/wolfxl/calc/_evaluator.py Added formulas library detection, fallback mechanism with compilation caching, parameter resolution, and result normalization
Comments suppressed due to low confidence (1)

python/wolfxl/calc/_evaluator.py:292

  • The _compiled_cache dictionary is never cleared when load() is called. If a workbook is reloaded with different formulas, the cache could contain stale compiled functions from the previous workbook. Consider clearing the cache in the load() method alongside _cell_values and _graph.
    def load(self, workbook: Workbook) -> None:
        """Scan workbook, store cell values, build dependency graph."""
        self._cell_values.clear()
        self._graph = DependencyGraph()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if len(args) < 1 or len(args) > 2:
raise ValueError("LEFT requires 1 or 2 arguments")
text = _coerce_string(args[0])
num_chars = int(_coerce_numeric([args[1]])[0]) if len(args) > 1 else 1
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LEFT function doesn't handle negative num_chars values, which would cause unexpected behavior. In Excel, LEFT with a negative number of characters returns a #VALUE! error. Consider adding validation to check if num_chars is negative and raise an appropriate error.

Suggested change
num_chars = int(_coerce_numeric([args[1]])[0]) if len(args) > 1 else 1
num_chars = int(_coerce_numeric([args[1]])[0]) if len(args) > 1 else 1
if num_chars < 0:
raise ValueError("LEFT: num_chars must be non-negative")

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 73831a1. LEFT now returns #VALUE\! for negative num_chars, with test.

nums = _coerce_numeric(args)
if len(nums) != 2:
raise ValueError("POWER: non-numeric argument")
return nums[0] ** nums[1]
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The POWER function doesn't handle the edge case of negative base with a fractional exponent, which would raise a ValueError in Python (complex result). Excel returns #NUM! error in this case. Consider adding validation to check for this condition and return an appropriate error value.

Suggested change
return nums[0] ** nums[1]
base, exponent = nums[0], nums[1]
# Excel returns a numeric error (#NUM!) when raising a negative base
# to a non-integer exponent. Mimic this by rejecting such inputs
# instead of returning a complex result.
if base < 0 and not float(exponent).is_integer():
raise ValueError("POWER: invalid for negative base with non-integer exponent")
return base ** exponent

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 73831a1. POWER now returns #NUM! for negative base with fractional exponent. Test added.

Comment on lines 171 to 172
n_rows = abs(end_row - start_row) + 1
n_cols = abs(end_col - start_col) + 1
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The range_shape function uses abs() to calculate n_rows and n_cols, which means it would work correctly even if the range is specified in reverse order (e.g., C3:A1). However, this is inconsistent with the expand_range function which normalizes the order explicitly using min/max. While this works, consider using the same pattern (min/max) for consistency with the rest of the codebase.

Suggested change
n_rows = abs(end_row - start_row) + 1
n_cols = abs(end_col - start_col) + 1
top_row = min(start_row, end_row)
bottom_row = max(start_row, end_row)
left_col = min(start_col, end_col)
right_col = max(start_col, end_col)
n_rows = bottom_row - top_row + 1
n_cols = right_col - left_col + 1

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 73831a1. Switched to min/max for consistency with expand_range().

Comment on lines +631 to +640
if ':' in param:
# Range parameter - resolve to numpy array
# Qualify with sheet name for range_shape parsing
qualified = param if '!' in param else f"{sheet}!{param}"
values = self._resolve_range(param, sheet)
flat = np.array([v if v is not None else 0 for v in values])
n_rows, n_cols = range_shape(qualified)
if n_cols > 1 and flat.size == n_rows * n_cols:
flat = flat.reshape(n_rows, n_cols)
args.append(flat)
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When converting None values to 0 for numpy arrays (line 636), this could lead to incorrect results for functions that should distinguish between empty cells and zeros. For example, COUNTA should count non-empty cells, but this conversion would make None cells appear as zeros. Consider preserving None values or using numpy's nan for missing values, depending on what the formulas library expects.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional: the formulas library requires numpy arrays with numeric values. The None→0 coercion is scoped to the formulas fallback path only - builtin functions like COUNTA handle None directly. Using np.nan would break formulas lib function signatures that expect float arrays.

Comment on lines 146 to 216
class TestBuiltinLeft:
def test_basic(self) -> None:
wb = wolfxl.Workbook()
ws = wb.active
ws["A1"] = "Hello World"
ws["B1"] = '=LEFT(A1,5)'
ev = WorkbookEvaluator()
ev.load(wb)
results = ev.calculate()
assert results["Sheet!B1"] == "Hello"

def test_default_one_char(self) -> None:
wb = wolfxl.Workbook()
ws = wb.active
ws["A1"] = "Hello"
ws["B1"] = '=LEFT(A1)'
ev = WorkbookEvaluator()
ev.load(wb)
results = ev.calculate()
assert results["Sheet!B1"] == "H"


class TestBuiltinRight:
def test_basic(self) -> None:
wb = wolfxl.Workbook()
ws = wb.active
ws["A1"] = "Hello World"
ws["B1"] = '=RIGHT(A1,5)'
ev = WorkbookEvaluator()
ev.load(wb)
results = ev.calculate()
assert results["Sheet!B1"] == "World"


class TestBuiltinMid:
def test_basic(self) -> None:
wb = wolfxl.Workbook()
ws = wb.active
ws["A1"] = "Hello World"
ws["B1"] = '=MID(A1,7,5)'
ev = WorkbookEvaluator()
ev.load(wb)
results = ev.calculate()
assert results["Sheet!B1"] == "World"


class TestBuiltinLen:
def test_basic(self) -> None:
wb = wolfxl.Workbook()
ws = wb.active
ws["A1"] = "Hello"
ws["B1"] = '=LEN(A1)'
ev = WorkbookEvaluator()
ev.load(wb)
results = ev.calculate()
assert results["Sheet!B1"] == 5.0


class TestBuiltinConcatenate:
def test_basic(self) -> None:
wb = wolfxl.Workbook()
ws = wb.active
ws["A1"] = "Hello"
ws["A2"] = " "
ws["A3"] = "World"
ws["B1"] = '=CONCATENATE(A1,A2,A3)'
ev = WorkbookEvaluator()
ev.load(wb)
results = ev.calculate()
assert results["Sheet!B1"] == "Hello World"

Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage for negative num_chars in LEFT, RIGHT, and MID functions. These edge cases should be tested to ensure the functions properly handle or reject invalid inputs, as noted in the function implementation issues.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 73831a1. Added edge case tests for LEFT(-1), MID(0,.), MID(.,-1), and POWER(-1, 0.5).

Comment on lines +593 to +594
import formulas as fm
import numpy as np
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formulas library and numpy are imported inside the _formulas_fallback method rather than at module level. While this lazy importing pattern is intentional to avoid dependencies when the formulas library is not installed, it means these imports will be executed on every call to _formulas_fallback. Consider moving the imports to the top of the method or caching the imported modules as class attributes after the first import.

Suggested change
import formulas as fm
import numpy as np
# Lazily import and cache the optional formulas and numpy modules
fm = getattr(self, "_formulas_module", None)
np = getattr(self, "_numpy_module", None)
if fm is None or np is None:
try:
import formulas as _fm # type: ignore[import-not-found]
import numpy as _np
except ImportError:
logger.debug(
"formulas fallback requested, but 'formulas' and/or 'numpy' "
"are not installed"
)
return None
self._formulas_module = fm = _fm
self._numpy_module = np = _np

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lazy import is intentional - formulas and numpy are optional extras (wolfxl[calc]). This method only runs as a fallback for functions without builtins. Module-level caching would add complexity for marginal benefit since Python already caches imported modules in sys.modules.

Comment on lines +9 to +10
import pytest

Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pytest import on line 9 is unused. Consider removing it or adding tests that use pytest features like parametrize or raises.

Suggested change
import pytest

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved by 73831a1 - pytest is now used by the _requires_formulas skip marker.

text = _coerce_string(args[0])
start = int(_coerce_numeric([args[1]])[0])
num_chars = int(_coerce_numeric([args[2]])[0])
# Excel MID is 1-indexed
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MID function doesn't validate that start and num_chars are positive. In Excel, MID with start < 1 returns #VALUE! error, and num_chars < 0 also returns #VALUE!. The current implementation would silently handle negative values incorrectly (e.g., start=0 would access text[-1]).

Suggested change
# Excel MID is 1-indexed
# Excel MID is 1-indexed; start must be >= 1 and num_chars >= 0
if start < 1 or num_chars < 0:
return "#VALUE!"

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 73831a1. MID now returns #VALUE\! for start < 1 or num_chars < 0. Two edge case tests added.

Comment on lines +95 to +96


Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage for POWER function with negative base and fractional exponent, which would cause a ValueError in the current implementation. This edge case should be tested to verify the expected error handling behavior.

Suggested change
def test_negative_base_fractional_exponent(self) -> None:
wb = wolfxl.Workbook()
ws = wb.active
ws["A1"] = -4
ws["B1"] = "=POWER(A1,0.5)"
ev = WorkbookEvaluator()
ev.load(wb)
with pytest.raises(ValueError):
ev.calculate()

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 73831a1. Added POWER(-1, 0.5) test that asserts #NUM\! return.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 83aede1f2e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

results = ev.calculate()
# Expected: ~-1073.64
val = results["Sheet!A1"]
assert val is not None, "PMT formula returned None - formulas lib not available?"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Skip formulas-integration assertions without optional dependency

These tests unconditionally assert that formulas-backed functions return non-None, but the default CI job installs wolfxl without extras (.github/workflows/ci.yml installs wolfxl, not wolfxl[calc]), so formulas is absent and this suite fails even when core code is correct. Please guard this module (or the formulas-only cases) with pytest.importorskip("formulas") or an equivalent marker so the default test matrix remains green.

Useful? React with 👍 / 👎.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 73831a1. Added @_requires_formulas skip marker to 4 formulas-dependent test classes. CI now passes without wolfxl[calc] extra.

nums = _coerce_numeric(args)
if len(nums) != 2:
raise ValueError("POWER: non-numeric argument")
return nums[0] ** nums[1]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject complex results from POWER for invalid real inputs

POWER currently returns nums[0] ** nums[1] directly, which yields a complex number for cases like POWER(-1, 0.5). The evaluator and protocol are built around scalar Excel-like values (int/float/str/bool/None), so storing a complex here can silently break downstream formulas (e.g., arithmetic paths that only accept int|float will start returning None). This should be converted to an Excel-style error path (e.g., raise and surface #NUM!) instead of returning complex values.

Useful? React with 👍 / 👎.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 73831a1. POWER now guards against complex results and returns #NUM\!.

…CH, XLOOKUP, CHOOSE, SUMIF, SUMIFS, COUNTIF, COUNTIFS)

Adds 8 new builtin functions for lookup and conditional aggregation,
bringing the builtin count from 25 to 33 and the whitelist from 39 to
43. These are the most commonly used Excel functions in financial models
and are required for LRBench-Agent formula evaluation.

Key changes:
- RangeValue dataclass: shape-aware 2D range container that preserves
  (n_rows, n_cols) metadata while staying iterable for backward compat
- & string concatenation operator: enables dynamic criteria like ">"&B1
- Criteria matching engine: shared by SUMIF/SUMIFS/COUNTIF/COUNTIFS,
  supports operators (>100), wildcards (a*), and exact match
- 43 new tests covering all builtins, backward compat, and perturbation
  propagation through INDEX/MATCH and SUMIF chains

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@wolfiesch wolfiesch changed the title feat(calc): formulas library fallback + 10 new builtins feat(calc): formulas fallback + 18 new builtins (lookup & conditional) Feb 20, 2026
- POWER: return #NUM! for negative base with fractional exponent
  instead of leaking complex numbers (Copilot #2, Codex #11)
- LEFT: return #VALUE! for negative num_chars (Copilot #1)
- MID: return #VALUE! for start < 1 or num_chars < 0 (Copilot #8)
- range_shape: use min/max instead of abs() for consistency with
  expand_range (Copilot #3)
- Add @_requires_formulas skip marker to formulas-dependent test
  classes so CI passes without wolfxl[calc] extra (Codex #10)
- Add 4 edge case tests: POWER(-1,0.5), LEFT(-1), MID(0,..), MID(.,-1)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@wolfiesch wolfiesch merged commit 679b80a into main Feb 20, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant